app/packages/relay/src/fragments/datasetAppConfigFragment.ts (1)

8-11: Add type definitions for include/exclude fields

While the structure is logical for visibility filtering, consider adding type definitions for the include and exclude fields to ensure type safety and better IDE support.

    defaultVisibilityLabels {
-     include
-     exclude
+     include: [String!]
+     exclude: [String!]
    }

app/packages/looker/src/worker/shared.ts (1)

3-3: Add JSDoc comments for the render status type

Consider adding documentation to explain the purpose and meaning of each render status state for better maintainability.

+/**
+ * Represents the rendering lifecycle of a dense label
+ * @type {null} Initial state before rendering begins
+ * @type {"painting"} Label is currently being rendered
+ * @type {"painted"} Label has been rendered successfully
+ * @type {"decoded"} Label data has been decoded but not yet rendered
+ */
export type DenseLabelRenderStatus = null | "painting" | "painted" | "decoded";

app/packages/looker/src/worker/resolve-color.ts (1)

33-42: Enhance promise error handling

The promise creation lacks error handling. Consider adding error handling and timeout mechanism.

-          seedPromises[key] = new Promise((resolve) => {
+          seedPromises[key] = new Promise((resolve, reject) => {
+            const timeout = setTimeout(() => {
+              delete seedRequests[key];
+              delete seedPromises[key];
+              reject(new Error('Color resolution timeout'));
+            }, 5000);
             seedRequests[key] = resolve;
             postMessage({
               method: "requestColor",
               key,
               seed,
               pool,
             });
+            return () => clearTimeout(timeout);
           });

app/packages/core/src/components/Sidebar/useOnSidebarSelectionChange.ts (2)

23-60: Optimize effect performance and add cleanup

The effect has several optimization opportunities:

1. The Set operations could be memoized
2. Missing cleanup function could lead to memory leaks
3. Consider using useMemo for currentActiveLabelFields

+ const currentActiveLabelFields = useMemo(
+   () => new Set(activeLabelFieldsValue),
+   [activeLabelFieldsValue]
+ );

  useEffect(() => {
    const thisPageActiveFields = sidebarTracker.get(gridPageValue);

-   const currentActiveLabelFields = new Set(activeLabelFieldsValue);

    if (currentActiveLabelFields.size === 0) {
      return;
    }

    // ... rest of the effect

+   return () => {
+     // Cleanup any subscriptions or pending state updates
+   };
  }, [activeLabelFieldsValue, sidebarTracker]);

---

15-17: Consider using useLayoutEffect for ref updates

Updating refs in render phase could lead to inconsistencies. Consider moving the ref update to useLayoutEffect.

  const gridPageValueRef = useRef(gridPageValue);

- gridPageValueRef.current = gridPageValue;
+ useLayoutEffect(() => {
+   gridPageValueRef.current = gridPageValue;
+ }, [gridPageValue]);

app/packages/utilities/src/schema.ts (1)

70-92: Add JSDoc documentation for the enhanced function signature.

The function has been enhanced with new parameters, but lacks documentation explaining their purpose and usage.

Add this documentation above the function:

+/**
+ * Returns fields with specified embedded document type(s).
+ * @param schema The schema to search through
+ * @param embeddedDocType Single or array of embedded document types to match
+ * @param shouldRecurse Whether to recursively search nested fields (default: true)
+ * @returns Array of matching fields
+ */
 export function getFieldsWithEmbeddedDocType(

app/packages/core/src/components/Grid/useRefreshers.ts (1)

100-109: Consider debouncing the refresh operation.

Refreshing all lookers on every label toggle could impact performance if there are many lookers. Consider implementing debouncing to prevent rapid consecutive refreshes.

+import { debounce } from 'lodash';
+
 const gridLabelsToggleTracker = useOnSidebarSelectionChange({ modal: false });
+const debouncedRefresh = useMemo(
+  () =>
+    debounce(() => {
+      lookerStoreRef.current?.forEach((looker) => {
+        looker.refreshSample();
+      });
+    }, 250),
+  []
+);
 
 useEffect(() => {
-  lookerStoreRef.current?.forEach((looker) => {
-    looker.refreshSample();
-  });
+  debouncedRefresh();
+  return () => {
+    debouncedRefresh.cancel();
+  };
 }, [gridLabelsToggleTracker]);

app/packages/looker/src/overlays/base.ts (1)

77-77: Add JSDoc documentation for the cleanup method.

The new setTargetsToNull parameter needs documentation to explain its purpose and when it should be set to true/false.

Add this documentation above the method:

+/**
+ * Cleans up resources used by the overlay.
+ * @param setTargetsToNull When true, sets target references to null for garbage collection
+ */
 cleanup?(setTargetsToNull: boolean): void;

app/packages/looker/src/worker/disk-overlay-decoder.ts (1)

134-135: Consider using proper type definition instead of type assertion.

The type assertion to DenseLabelRenderStatus suggests that the label object's type definition might need improvement.

-  label.renderStatus = "decoded" as DenseLabelRenderStatus;
+  label.renderStatus = "decoded";

And update the label parameter type:

-  label: Record<string, any>,
+  label: Record<string, any> & { renderStatus?: DenseLabelRenderStatus },

app/packages/looker/src/overlays/detection.ts (1)

264-269: LGTM! Consider adding JSDoc.

The implementation is correct and maintains backward compatibility. Consider adding JSDoc to document the purpose of the setTargetsToNull parameter.

+/**
+ * Cleans up resources associated with the detection overlay.
+ * @param setTargetsToNull - When true, sets the label mask to null after cleanup
+ */
 public cleanup(setTargetsToNull = false): void {

app/packages/looker/src/worker/painter.ts (1)

33-34: LGTM! Consider using an enum for render status.

The implementation of render status tracking is consistent across all methods. For better type safety and maintainability, consider using an enum for the render status values.

enum RenderStatus {
  PAINTING = "painting",
  PAINTED = "painted",
  DECODED = "decoded",
  NONE = null
}

Also applies to: 146-147, 162-163, 193-194, 274-275, 407-408

app/packages/state/src/recoil/selectors.ts (1)

145-152: LGTM! Consider handling potential undefined values.

The selector implementation is clean and type-safe. Consider handling the case where datasetAppConfig might be undefined.

 export const defaultVisibilityLabels =
   selector<State.DefaultVisibilityLabelsConfig>({
     key: "defaultVisibilityLabels",
     get: ({ get }) => {
-      return get(datasetAppConfig)
+      const config = get(datasetAppConfig);
+      return config
         .defaultVisibilityLabels as State.DefaultVisibilityLabelsConfig | null;
     },
   });

app/packages/looker/src/worker/index.ts (1)

Line range hint 310-323: Consider documenting the 3D labels exception.

The code correctly processes all 3D labels regardless of active paths, but this behavior should be documented.

-    // we process all 3d labels regardless of active paths
+    // We process all 3D labels regardless of active paths because
+    // 3D label rendering has different optimization constraints
+    // and dependencies compared to 2D labels.
     process3DLabels(schema, sample);

fiftyone/core/odm/dataset.py (1)

518-519: Documentation needs clarification.

The docstring for default_visibility_labels should specify the expected dictionary structure (include/exclude lists) to match the GraphQL schema.

-        default_visibility_labels (None): config with include and exclude lists
-            of labels to render by default in the App
+        default_visibility_labels (None): a dictionary containing 'include' and 'exclude'
+            lists of label field names to control their default visibility in the App.
+            Example::
+                {
+                    "include": ["detections", "classifications"],
+                    "exclude": ["segmentations"]
+                }

app/schema.graphql (1)

332-335: LGTM! Consider adding descriptions.

The FieldVisibilityConfig type is well-structured. Consider adding descriptions to document the purpose of each field.

 type FieldVisibilityConfig {
+  """
+  List of label field names to show by default
+  """
   include: [String!]
+  """
+  List of label field names to hide by default
+  """
   exclude: [String!]
 }

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

app/packages/looker/src/lookers/abstract.ts (1)

721-725: Enhance clarity of the setTargetsToNull parameter

The parameter name setTargetsToNull in the cleanOverlays method might be unclear to other developers. Consider renaming it to something more descriptive, such as disposeOverlays or resetOverlayTargets, to better convey its purpose and improve code readability.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

app/packages/state/src/recoil/schema.ts (1)

325-378: Well-structured implementation of label visibility logic.

The selector efficiently handles different visibility configurations using Set operations. However, the Set operations could be optimized further.

Consider this optimization for the Set operations:

-    const includeList = new Set(defaultVisibleLabelsConfig.include);
-    const excludeList = new Set(defaultVisibleLabelsConfig.exclude);
-    // resolved = set(include) - set(exclude)
-    const resolved = new Set(
-      [...includeList].filter((x) => !excludeList.has(x))
-    );
-    return allLabels.filter((label) => resolved.has(label));
+    // Pre-filter allLabels against include list first
+    const excludeSet = new Set(defaultVisibleLabelsConfig.exclude);
+    return allLabels
+      .filter((label) => defaultVisibleLabelsConfig.include.includes(label))
+      .filter((label) => !excludeSet.has(label));

This optimization:

1. Reduces memory usage by creating only one Set
2. Avoids unnecessary Set creation for the include list
3. Maintains the same functionality with better performance

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

app/packages/core/src/components/Sidebar/useShouldReloadSample.ts (1)

9-9: Consider memory management for gridActivePathsLUT

The global Map instance could potentially grow unbounded as new Looker IDs are added. Consider implementing a cleanup mechanism or using a size-limited LRU cache.

-export const gridActivePathsLUT = new Map<LookerId, CachedLabels>();
+import LRU from "lru-cache";
+export const gridActivePathsLUT = new LRU<LookerId, CachedLabels>({
+  max: 1000, // Adjust based on expected concurrent Looker instances
+});

app/packages/core/src/components/Sidebar/syncAndCheckRefreshNeeded.ts (1)

44-48: Consider optimizing Set creation

The spread operator with Set creation could be optimized for performance.

-    new Set([...(thisLookerActiveFields ?? []), ...currentActiveLabelFields])
+    new Set(function*() {
+      if (thisLookerActiveFields) yield* thisLookerActiveFields;
+      yield* currentActiveLabelFields;
+    }())

app/packages/core/src/components/Grid/useSelect.ts (1)

38-41: Enhance comment clarity

The current comment could be more descriptive about why this refresh is necessary.

-        // rerender looker if active fields have changed and have never been rendered before
+        // Trigger a looker refresh when newly activated label fields are detected,
+        // ensuring they are properly rendered in the viewport

app/packages/core/src/components/Sidebar/syncAndCheckRefreshNeeded.test.ts (2)

17-18: Fix typo in comment

There's a typo in the comment.

-    // lut shoiuldn't be modified
+    // lut shouldn't be modified

---

4-96: LGTM! Comprehensive test coverage

The test suite is well-structured and covers important edge cases:

• Empty fields
• New looker scenarios
• Subset of fields
• Repeated calls

Consider adding a test case for concurrent modifications to ensure thread safety.

app/packages/core/src/components/Grid/Grid.tsx (2)

Line range hint 179-179: Remove debug console.log statement

The console.log statement should be removed before merging to production.

-        console.log(">>>painting ", `${prefix ?? ""}${field}`);

---

50-57: Add dependencies array to useRecoilCallback

The useRecoilCallback hook should include a dependencies array as the second argument to prevent unnecessary recreations of the callback.

  const getCurrentActiveLabelFields = useRecoilCallback(
    ({ snapshot }) =>
      () => {
        return snapshot
          .getLoadable(fos.activeLabelFields({ modal: false }))
          .getValue();
      }
-  );
+  , []);

app/packages/looker/src/worker/index.ts (1)

Line range hint 773-783: Improve error handling in worker message posting

The catch block could be more robust by logging the error before retrying.

    } catch (error) {
      // rarely we'll get a DataCloneError
      // if one of the buffers is detached and we didn't catch it
      // try again without transferring the buffers (copying them)
      if (error.name === "DataCloneError") {
+       console.warn("DataCloneError encountered, retrying without buffer transfer", error);
        labelsWorker.postMessage(workerArgs);
      } else {
+       console.error("Unexpected error in worker message posting", error);
        throw error;
      }
    }

app/packages/looker/src/lookers/abstract.ts (1)

536-543: Improve the TODO comment with more context

The TODO comment should provide more context about the issue and potential solutions.

-  // todo: sometimes instance in spotlight?.updateItems() is defined but has no ref to sample
-  // this crashes the app. this is a bug and should be fixed
+  /**
+   * TODO: Fix crash in spotlight?.updateItems()
+   * Issue: Sometimes the instance exists but has no reference to sample
+   * Potential fixes:
+   * 1. Add null checks in updateItems
+   * 2. Ensure sample is always set before calling updateItems
+   * 3. Add error boundary to handle the crash gracefully
+   */

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

Learnt from: sashankaryal
PR: voxel51/fiftyone#5356
File: app/packages/looker/src/lookers/abstract.ts:49-50
Timestamp: 2025-01-09T23:46:50.918Z
Learning: The `UPDATING_SAMPLES_IDS` set in `app/packages/looker/src/lookers/abstract.ts` is intentionally module-scoped to prevent concurrent updates of the same sample across different instances of AbstractLooker.

app/packages/core/src/components/Grid/Grid.tsx (1)

115-126: Consider memoizing the active label fields initialization.

While the implementation is correct, consider memoizing the currentActiveLabelFields value to prevent unnecessary recomputation during frequent renders.

+ const memoizedActiveLabelFields = useMemo(
+   () => getCurrentActiveLabelFields(),
+   [getCurrentActiveLabelFields]
+ );

  // initialize active paths tracker
- const currentActiveLabelFields = getCurrentActiveLabelFields();
  if (
-   currentActiveLabelFields &&
+   memoizedActiveLabelFields &&
    !gridActivePathsLUT.has(id.description)
  ) {
    gridActivePathsLUT.set(
      id.description,
-     new Set(currentActiveLabelFields)
+     new Set(memoizedActiveLabelFields)
    );
  }

app/packages/utilities/src/schema.ts (2)

70-92: Consider adding type guards for the embeddedDocType parameter.

The function now accepts both string and string[] types, but there's no type guard to ensure type safety at runtime.

 export function getFieldsWithEmbeddedDocType(
   schema: Schema,
-  embeddedDocType: string | string[],
+  embeddedDocType: string | readonly string[],
   shouldRecurse = true
 ): Field[] {
   const result: Field[] = [];
+  const types = Array.isArray(embeddedDocType) ? embeddedDocType : [embeddedDocType];
 
   function recurse(schema: Schema) {
     for (const field of Object.values(schema ?? {})) {
-      if (Array.isArray(embeddedDocType)) {
-        if (embeddedDocType.includes(field.embeddedDocType)) {
-          result.push(field);
-        }
-      } else if (field.embeddedDocType === embeddedDocType) {
+      if (types.includes(field.embeddedDocType)) {
         result.push(field);
       }
       if (field.fields && shouldRecurse) {
         recurse(field.fields);
       }
     }
   }
 
   recurse(schema);
   return result;
 }

---

116-124: Add JSDoc comments for the new function.

The function's purpose and return value should be documented for better maintainability.

+/**
+ * Returns an array of field names that represent dense labels in the schema.
+ * Dense labels are fields with embedded document types defined in DENSE_LABEL_EMBEDDED_DOC_TYPES.
+ * @param schema - The schema to search for dense labels
+ * @returns An array of field names representing dense labels
+ */
 export function getDenseLabelNames(schema: Schema): string[] {
   const denseLabels = getFieldsWithEmbeddedDocType(
     schema,
     DENSE_LABEL_EMBEDDED_DOC_TYPES,
     false
   );
 
   return denseLabels.map((label) => label.name);
 }

app/packages/state/src/recoil/schema.ts (1)

325-378: Optimize Set operations for better performance.

The implementation is logically correct but could be optimized when handling include/exclude lists.

 // is in both include and exclude
-const includeList = new Set(defaultVisibleLabelsConfig.include);
-const excludeList = new Set(defaultVisibleLabelsConfig.exclude);
-// resolved = set(include) - set(exclude)
-const resolved = new Set(
-  [...includeList].filter((x) => !excludeList.has(x))
-);
-return allLabels.filter((label) => resolved.has(label));
+const excludeSet = new Set(defaultVisibleLabelsConfig.exclude);
+return allLabels.filter(
+  (label) =>
+    defaultVisibleLabelsConfig.include.includes(label) && !excludeSet.has(label)
+);

app/packages/looker/src/worker/resolve-color.ts (1)

19-20: Consider using Map for better type safety.

The use of plain objects for storing requests and promises could benefit from using Map for better type safety and built-in key management.

-  const requests = {};
-  const promises = {};
+  const requests = new Map<number, Record<string | number, (color: string) => void>>();
+  const promises = new Map<number, Record<string | number, Promise<string>>>();

app/packages/looker/src/worker/index.ts (1)

311-311: Document 3D labels processing behavior.

Add a comment explaining why 3D labels are processed regardless of activePaths status.

-    // we process all 3d labels regardless of active paths
+    // 3D labels are always processed regardless of active paths because they are
+    // essential for the scene geometry and cannot be selectively rendered

app/packages/looker/src/worker/disk-overlay-decoder.ts (1)

134-135: Consider improving type safety and error handling.

The current implementation has a few areas for improvement:

1. Type casting could be replaced with proper type definition
2. Consider adding error handling for the race condition mentioned in the PR objectives

Apply this diff to improve type safety:

-  label.renderStatus = "decoded" as DenseLabelRenderStatus;
+  label.renderStatus = "decoded";
+  type LabelWithRenderStatus = typeof label & { renderStatus: DenseLabelRenderStatus };

Also, consider adding a check to prevent race conditions:

+  if (label.renderStatus === "painting") {
+    // Skip updating if already being processed
+    return;
+  }
   label.renderStatus = "decoded" as DenseLabelRenderStatus;

app/packages/looker/src/lookers/abstract.ts (1)

49-50: Add documentation for UPDATING_SAMPLES_IDS.

While the module-scoped Set is intentional (as per retrieved learning), adding documentation would help future maintainers understand its purpose and scope.

+/**
+ * A shared Set to track sample IDs that are currently being updated across all instances
+ * of AbstractLooker. This is intentionally module-scoped to prevent concurrent updates
+ * of the same sample across different instances.
+ */
 const UPDATING_SAMPLES_IDS = new Set();

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

Learnt from: sashankaryal
PR: voxel51/fiftyone#5356
File: app/packages/looker/src/lookers/abstract.ts:49-50
Timestamp: 2025-01-09T23:46:50.918Z
Learning: The `UPDATING_SAMPLES_IDS` set in `app/packages/looker/src/lookers/abstract.ts` is intentionally module-scoped to prevent concurrent updates of the same sample across different instances of AbstractLooker.

app/packages/core/src/components/Modal/use-looker.ts (1)

59-67: Consider debouncing the refresh operation.

While the implementation is correct, rapid changes to lookerOptions.activePaths could trigger multiple refreshes in quick succession. This might be related to the race condition mentioned in the PR objectives regarding sample recoloring.

Consider adding debounce:

  useEffect(() => {
    if (!looker) {
      return;
    }

+   const debouncedRefresh = debounce(() => {
      if (shouldRefresh(id)) {
        looker?.refreshSample();
      }
+   }, 150);
+
+   debouncedRefresh();
+   return () => debouncedRefresh.cancel();
-   if (shouldRefresh(id)) {
-     looker?.refreshSample();
-   }
  }, [id, lookerOptions.activePaths, looker]);

app/packages/core/src/components/Sidebar/useShouldReloadSample.ts (3)

6-10: Consider enhancing type safety.

While the types are correctly defined, they could be more specific:

-export type LookerId = string;
+export type LookerId = string & { readonly __brand: unique symbol };

This would prevent accidental assignment of any string as a LookerId.

---

12-28: Document the hook's behavior and return value.

The implementation is solid, but missing JSDoc documentation would help other developers understand the hook's purpose and usage.

Add documentation:

+/**
+ * Hook to manage and track changes in label visibility for samples
+ * @param {Object} params - Hook parameters
+ * @param {boolean} params.modal - Whether the hook is used in modal context
+ * @returns {Function} shouldRefresh - Function to check if a sample needs refreshing
+ */
export const useShouldReloadSampleOnActiveFieldsChange = ({
  modal,
}: {
  modal: boolean;
}) => {

---

33-41: Consider adding error handling for cleanup.

The cleanup logic is correct but could benefit from error handling to ensure robustness.

  useEffect(() => {
    return () => {
+     try {
        if (modal) {
          modalActivePathsLUT.clear();
        } else {
          gridActivePathsLUT.clear();
        }
+     } catch (error) {
+       console.error('Failed to clear lookup table:', error);
+     }
    };
  }, []);

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

app/packages/app/src/components/Analytics.tsx (1)

46-54: LGTM! Well-documented test environment handling.

The conditional check effectively disables analytics during Playwright tests, with clear comments explaining this exceptional case. The approach is pragmatic for improving test performance.

However, consider adding a type definition for the IS_PLAYWRIGHT window property to improve type safety.

+// Add to a global.d.ts file
+interface Window {
+  IS_PLAYWRIGHT?: boolean;
+}

e2e-pw/src/oss/fixtures/loader.ts (1)

130-135: LGTM! Clean implementation of test environment flag.

The initialization script is properly placed to ensure early setup of the IS_PLAYWRIGHT flag.

Consider adding a type definition instead of using @ts-ignore:

+// Add to a global.d.ts file
+interface Window {
+  IS_PLAYWRIGHT?: boolean;
+}

app/packages/core/src/components/QueryPerformanceToast.tsx (1)

80-88: LGTM, but consider adding type safety for the global variable.

The conditional check effectively prevents test delays. However, the window.IS_PLAYWRIGHT property should be properly typed.

Add type declaration for the global property:

declare global {
  interface Window {
    IS_PLAYWRIGHT?: boolean;
  }
}

app/packages/state/src/recoil/schema.ts (2)

325-378: Well-structured implementation with room for performance optimization.

The selector efficiently handles different visibility configurations using Set operations. However, we can optimize the array spread operations.

Consider this optimization:

-    const denseLabels = [...denseLabelsSamples, ...denseLabelsFrames];
-    const allLabels = [...allSampleLabels, ...allFrameLabels];
+    const denseLabels = denseLabelsSamples.concat(denseLabelsFrames);
+    const allLabels = allSampleLabels.concat(allFrameLabels);

---

325-378: Enhance code documentation for better maintainability.

While the logic is clear, adding JSDoc comments would improve maintainability.

Consider adding documentation:

+/**
+ * Selector that computes the list of visible labels based on configuration.
+ * 
+ * @returns {string[]} Array of visible label paths
+ * 
+ * Configuration cases:
+ * 1. No config: Hide dense labels
+ * 2. Include only: Show only included labels
+ * 3. Exclude only: Show all except excluded
+ * 4. Both: Show (included - excluded)
+ */
 export const defaultVisibleLabels = selector({

app/packages/core/src/components/Sidebar/useShouldReloadSample.ts (1)

12-44: Add TypeScript return type annotation to the hook.

The hook implementation follows React best practices, but could benefit from explicit TypeScript return type annotation for better type safety and documentation.

-export const useShouldReloadSampleOnActiveFieldsChange = ({
+export const useShouldReloadSampleOnActiveFieldsChange = ({
   modal,
 }: {
   modal: boolean;
-}) => {
+}): (id: string) => boolean => {

app/packages/looker/src/worker/index.ts (2)

Line range hint 52-71: Enhance documentation for the activePaths parameter.

While the function steps are well documented, consider adding a description of the activePaths parameter and its role in the processing logic.

---

96-121: Refactor duplicated active paths check into a helper function.

The active paths check logic is duplicated. Consider extracting it into a helper function to improve maintainability and reduce code duplication.

+const isFieldActive = (field: string, prefix: string | null, activePaths: string[]): boolean => {
+  return activePaths.length && activePaths.includes(`${prefix ?? ""}${field}`);
+};

 // Then use it in both places:
-if (
-  activePaths.length &&
-  activePaths.includes(`${prefix ?? ""}${field}`)
-) {
+if (isFieldActive(field, prefix, activePaths)) {

Also applies to: 174-191

app/packages/looker/src/lookers/abstract.ts (4)

49-50: Add documentation for the module-scoped Set.

While the module-scoped Set is intentional (as confirmed by retrieved learning), consider adding documentation to explain its purpose in preventing concurrent updates across different instances.

+/**
+ * A shared Set to track sample IDs that are currently being updated across all instances
+ * of AbstractLooker. This is intentionally module-scoped to prevent concurrent updates
+ * of the same sample across different instances, avoiding potential race conditions.
+ */
 const UPDATING_SAMPLES_IDS = new Set();

---

522-532: Add error handling for sample updates.

Consider adding error handling and a maximum retry count to prevent infinite retry loops in case of persistent issues.

+private maxRetries = 3;
+private retryCount = new Map<string, number>();

 updateSample(sample: Sample) {
   const id = sample.id ?? sample._id;

   if (UPDATING_SAMPLES_IDS.has(id)) {
+    const currentRetries = this.retryCount.get(id) ?? 0;
+    if (currentRetries >= this.maxRetries) {
+      console.error(`Max retries reached for sample ${id}`);
+      this.retryCount.delete(id);
+      return;
+    }
+    this.retryCount.set(id, currentRetries + 1);
     UPDATING_SAMPLES_IDS.delete(id);
     this.cleanOverlays(true);
     queueMicrotask(() => this.updateSample(sample));
     return;
   }

+  this.retryCount.delete(id);
   UPDATING_SAMPLES_IDS.add(id);

---

536-543: Address the TODO comment regarding potential crashes.

The TODO comment indicates a bug where the instance in spotlight?.updateItems() might have an undefined sample reference, leading to crashes.

Would you like me to help investigate this issue and propose a solution? I can help create a GitHub issue to track this bug.

---

726-729: Add JSDoc for the setTargetsToNull parameter.

Consider adding documentation to explain the purpose and impact of the setTargetsToNull parameter.

+/**
+ * Cleans up overlays and optionally nullifies their targets.
+ * @param setTargetsToNull When true, overlay targets will be set to null during cleanup
+ */
 protected cleanOverlays(setTargetsToNull = false) {

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

Learnt from: sashankaryal
PR: voxel51/fiftyone#5356
File: app/packages/looker/src/lookers/abstract.ts:49-50
Timestamp: 2025-01-09T23:46:50.918Z
Learning: The `UPDATING_SAMPLES_IDS` set in `app/packages/looker/src/lookers/abstract.ts` is intentionally module-scoped to prevent concurrent updates of the same sample across different instances of AbstractLooker.

app/packages/state/src/labelsVisibility.ts (3)

27-27: Remove console.log statement.

Production code should not contain console.log statements.

-    console.log("all", all);

---

19-57: Optimize label filtering using Set operations.

The current implementation uses array operations which have O(n²) complexity. Using Set operations would improve performance.

-  const allLabels = [...allSampleLabels, ...allFrameLabels];
+  const allLabelsSet = new Set([...allSampleLabels, ...allFrameLabels]);
+  const denseLabelsSet = new Set(denseLabels);
 
   if (
     !defaultVisibleLabelsConfig?.include &&
     !defaultVisibleLabelsConfig?.exclude
   ) {
-    const all = allLabels.filter((label) => !denseLabels.includes(label));
-    return all;
+    return [...allLabelsSet].filter((label) => !denseLabelsSet.has(label));
   }
 
   if (
     defaultVisibleLabelsConfig?.include &&
     !defaultVisibleLabelsConfig?.exclude
   ) {
-    return allLabels.filter((label) =>
-      defaultVisibleLabelsConfig.include.includes(label)
-    );
+    const includeSet = new Set(defaultVisibleLabelsConfig.include);
+    return [...allLabelsSet].filter((label) => includeSet.has(label));
   }
 
   if (
     !defaultVisibleLabelsConfig?.include &&
     defaultVisibleLabelsConfig?.exclude
   ) {
-    return allLabels.filter(
-      (label) => !defaultVisibleLabelsConfig.exclude.includes(label)
-    );
+    const excludeSet = new Set(defaultVisibleLabelsConfig.exclude);
+    return [...allLabelsSet].filter((label) => !excludeSet.has(label));
   }

---

3-12: Add JSDoc documentation.

Add comprehensive documentation to explain the function's purpose, parameters, and return value.

+/**
+ * Computes the list of labels that should be visible based on the provided schemas and configuration.
+ * @param sampleSchema - The schema for sample labels
+ * @param frameSchema - The schema for frame labels
+ * @param allSampleLabels - Array of all sample label names
+ * @param allFrameLabels - Array of all frame label names
+ * @param defaultVisibleLabelsConfig - Optional configuration for label visibility
+ * @param defaultVisibleLabelsConfig.include - Optional array of labels to include
+ * @param defaultVisibleLabelsConfig.exclude - Optional array of labels to exclude
+ * @returns Array of label names that should be visible
+ */
 export const computeDefaultVisibleLabels = (

app/packages/state/src/labelsVisibility.test.ts (1)

40-54: Enhance test coverage with edge cases.

While the current test cases cover the main scenarios, consider adding tests for:

1. Empty arrays in include/exclude configs
2. Non-existent labels in include/exclude configs
3. Overlapping labels in include/exclude configs
4. Empty input label arrays

Here's an example test case for empty configs:

it("handles empty include/exclude arrays", () => {
  const allSampleLabels = ["detection", "classification"];
  const allFrameLabels = ["frames.detection"];

  const result = computeDefaultVisibleLabels(
    sampleSchemaMock,
    frameSchemaMock,
    allSampleLabels,
    allFrameLabels,
    { include: [], exclude: [] }
  );

  expect(result).toEqual([]);
});

Also applies to: 56-69, 71-84, 86-102

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro